Looking 👁️

Review — Linear dispatcher in fanout consumer (#239)

Thanks for this — it's a clean, well-tested addition and it closes a real UX gap. The ABCA-91 case (agent crashes at max_turns after shipping a PR, leaving the Linear requester with only a ❌ reaction and no metrics) is exactly the kind of "outcomes must stay inspectable" gap worth closing, and doing it platform-side so it fires even when the agent never reaches its own step-3 comment is the right call.

A few things I especially liked:

• The dispatcher reuses the existing NotificationChannel + CHANNEL_DEFAULTS + DISPATCHERS surface and mirrors Slack's channel_source gate and best-effort failure contract — same idiom, real per-channel substance, no copy-paste.
• Numerics are coerced at the DDB boundary with coerceNumericOrNull, matching the GitHub dispatcher.
• IAM grants are guarded behind optional props, so a deployment without Linear onboarding gets no dangling permissions.
• The mockDdbSend refactor from index-based assertions to command-type filtering is the correct defensive fix now that GitHub + Linear both hit the shared Get mock.
• Docs are in good shape — I ran node scripts/sync-starlight.mjs on the branch and the Starlight mirror is in sync, so CI's mutation guard should pass.

Verdict: Approve with nits. One item I'd like scoped down before merge, plus some polish.

Please address before merge

1. bedrock-agentcore:* action wildcard — least privilege (cdk/src/constructs/github-screenshot-integration.ts:195)

actions: ['bedrock-agentcore:*'],
resources: ['*'],

The handler only calls StartBrowserSession, StopBrowserSession, and the SigV4-presigned ConnectBrowserAutomationStream WSS endpoint. resources: ['*'] is defensible (ephemeral sessions have no stable ARN, and the IAM5 suppression documents it), but the action wildcard grants every AgentCore action (memory, runtime, gateway, identity, code-interpreter) we don't use. I see the commit history notes this was widened to unblock e2e with a "scope back down" follow-up — this is a good moment to do it:

actions: [
  'bedrock-agentcore:StartBrowserSession',
  'bedrock-agentcore:StopBrowserSession',
  'bedrock-agentcore:ConnectBrowserAutomationStream',
],

If ConnectBrowserAutomationStream can't be confirmed as the exact IAM action name (it wasn't in the public CLI list), CloudTrail's decoded deny message will have it. If it genuinely can't be resolved, keeping the wildcard is acceptable as long as the blocker is documented inline with a tracking issue rather than a bare "followup."

Non-blocking nits

2. Doc-vs-code mismatch on the ⚠️ frame (renderLinearFinalStatusComment JSDoc, fanout-task-events.ts). The comment keys case 2 on error_max_turns as an eventType, but error_max_turns is an agent_status, never an event_type — the real trigger is !isCompleted && prUrl != null (any terminal failure that still produced a PR). The test correctly passes event_type: 'task_failed'. Worth aligning the comment to the real condition.

3. classifyError returns UNKNOWN_CLASSIFICATION, not null, for unmatched non-empty messages (error-classifier.ts:359). The dispatcher's inline comment says it "returns null … the renderer falls back to a generic frame," but a failed task with any non-empty unmatched error_message will actually render ❌ Task failed: Unexpected error. Not a bug — just pick the intended behavior and align the comment with it.

4. The classifier title is dropped on the ⚠️ path. For the ABCA-91 case (max-turns + PR), classification.title ("Exceeded max turns") is computed but not rendered — the requester sees the PR but not why it stopped, which is the most useful context for the motivating issue. Consider appending it to the ⚠️ frame.

5. Log severity (dispatchToLinear): the missing_env and post_failed paths log at INFO, whereas the GitHub dispatcher's equivalent missing-env path is WARN. Since this comment is the only completion signal for the crash case, a misconfigured env var or revoked OAuth token failing every post is worth a WARN + error_id so it's alarmable. (The underlying linear-feedback.ts failures already WARN, so this is a backstop.)

6. Stale comment in routeEvent (fanout-task-events.ts): "at most 3 channels" — now 4 (slack/github/linear/email).

7. Output description leftover (agent.ts:883): one ScreenshotBucketName description still reads "Vercel-preview screenshots" after the de-Vercel-ize sweep. Cosmetic.

Tests

Coverage is solid and the suite passes locally (86 tests in fanout-task-events.test.ts). A few gaps the issue's acceptance criteria named that would be cheap to close:

• Missing LINEAR_WORKSPACE_REGISTRY_TABLE_NAME env path — the env is set at module load and never unset, so the deploy-misconfig safety valve is unexercised.
• renderLinearFinalStatusComment null-metric rendering — every fixture supplies non-null metrics, so the — fallbacks and formatDuration boundaries (<60s, exact-minute Nm) aren't covered. This is exactly the crash-before-metrics case the feature is built for; a small table-driven test calling the exported renderer directly would do it.
• error_max_turns without pr_url — the ⚠️↔❌ boundary (the prUrl != null flip) isn't asserted in that direction.

(The missing-OAuth-token graceful no-op, AC item 5, is covered at the helper layer in linear-feedback.test.ts — no need to duplicate it here.)

One small heads-up: since the PR is stacked on #241, the diff against main carries the full screenshot pipeline too — happy to review just the #239 delta if you retarget the base, otherwise no worries.

Nice work overall — scope the IAM action and this is good to go.

Reviewed with the help of automated agents (code-reviewer, silent-failure-hunter, pr-test-analyzer). 🤖

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@krokoko thanks for the review. All items addressed across two review-response commits.

Shipped on this branch

Blocking item (1) — bedrock-agentcore:* action wildcard (d8d9479)
Narrowed github-screenshot-integration.ts:195 to the three actions the handler actually calls: StartBrowserSession, StopBrowserSession, ConnectBrowserAutomationStream. Resource wildcard stays (sessions are ephemeral, no per-resource ARN); the cdk-nag IAM5 suppression already documents that.

Non-blocking nits (3280d2c, b84ce1e, d8d9479)

Test gaps (3280d2c)

• New test: LINEAR_WORKSPACE_REGISTRY_TABLE_NAME unset → WARN + skip (deploy-misconfig safety valve)
• New test: error_max_turns without pr_url renders ❌ not ⚠️ (the boundary in the other direction)
• New describe block: 8 direct tests of renderLinearFinalStatusComment covering null-metric fallbacks, formatDuration boundaries (<60s, exact-minute Nm, mixed Nm Ss), classifier-title rendering on ⚠️ and ❌ frames, and the no-trailing-colon when errorTitle is null

102 tests in fanout-task-events.test.ts (was 92), full suite still green.

Bonus from first dev smoke-test (b84ce1e)

After deploying to dev I noticed two near-duplicate signals on the Linear thread:

• ✅ comment carried PR: <url> while the agent's step-2 had already posted the same link → render PR URL only on ⚠️ now
• Agent step-3 "task completed" stacked next to the platform's structured ✅ → removed step 3 from the Linear-channel prompt contract; agent now posts start (1) + PR-opened (2), platform posts terminal ✅/⚠️/❌ (3)

You'd predicted this exact migration in your review ("the agent prompt can drop step 3 once the platform side is reliable").

Heads-up

PR is stacked on PR #241 — the diff against main carries the screenshot pipeline too. Happy to retarget the base for review purposes if it'd help; otherwise the #239 delta is the three commits 3ba880d, 3280d2c, b84ce1e, d8d9479.

Re-requesting review.